-
-
Notifications
You must be signed in to change notification settings - Fork 443
#1404 Added Message History to Serial Monitor #1562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was an error in the next method wherein the last value was being shown twice. It has been corrected with the latest commit. I have also downloaded the artifact and tested against it. |
I had started with the code in #1467 . The discussion there was about renaming the RingList to a different name as Ring implies cyclical behavior. Although HistoryStack was suggested as an alternative, I believed that was wrong. The functionality required traversal through the data structure and hence HistoryList was better name, The expected behavior should be as in Arduino 1.x . Let me know once you have tested it. |
Akos,
I believe there is still a bug in the way it behaves. I have not had a chance to get back to debugging the code.
Is the team getting close to a release?
Thanks,
Dwight
From: Akos Kitta ***@***.***>
Sent: Monday, October 17, 2022 11:52 AM
To: arduino/arduino-ide ***@***.***>
Cc: Dwight Fowler ***@***.***>; Mention ***@***.***>
Subject: Re: [arduino/arduino-ide] #1404 Added Message History to Serial Monitor (PR #1562)
Thank you, @nmzaheer<https://github.com/nmzaheer>. I will take a look soon. Can you tell me something about the expected behavior? Previously (#1467<#1467>), the stack was named as a ring, now it's a list. Shall I assert the monitor changes compared with IDE 1.x message history behavior?
—
Reply to this email directly, view it on GitHub<#1562 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AT4ZRZJGMPSTA4LFG4C7DFTWDVYZFANCNFSM6AAAAAARGH4ZOM>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
I will ensure this feature is part of the next patch release of IDE2. We plan to roll out the next version this month. Please bear with us. @dwightfowler-cd, if you do not plan to do follow-ups on #1467, please close it. Thanks for keeping the repo clean ❤️ |
@kittaakos Did you have a chance to test the behaviour? Do you have any feedback for me? |
It's working great. Thank you for your contribution. I believe the logic can be simplified. Let's remove the unnecessary state from the Please give it a try. If you like it, you can |
Thank you for testing. Your implementation looks much neater and simpler than what I had submitted. I am testing your implementation of HistoryList with this code. However, it skips the first entry when I'll get back to you with the changes. Edit previous(): string {
if (this.index === -1) {
this.index = this.items.length - 1;
return this.items[this.index];
}
if (this.hasPrevious) {
return this.items[--this.index];
}
return this.items[this.index];
} Looking forward to your feedback. |
- The implementation has been taken from @kittaakos repo https://github.com/kittaakos/arduino-ide/blob/d10de017362989b62d01991a33eaef9e71a6aec4/arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx - The previous method has been modified to ensure the first element instead of an empty string is returned if the index is at the beginning of the list. - The push method has been modified to check if the current command is same as the last command. If same then, it is not added to the list else it is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is looking good and the monitor history works as in IDE 1.x 👍
arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! It's looking good.
Motivation
Feature added for message history in Serial Monitor
Change description
Utilizes the RingList implementation of @dwightfowler-cd here with some corrections for traversal of the list
Other information
Reviewer checklist